Skip to content

Conversation

@mohammedahmed18
Copy link
Contributor

@mohammedahmed18 mohammedahmed18 commented Jul 9, 2025

PR Type

Feature


Description

  • Add LSP commands to verify and submit API key

  • Adapt API key retrieval for LSP quiet mode

  • Refactor LSP server init to defer optimizer setup

  • Quote API key export and clear cache on update


Changes diagram

flowchart LR
  F["apiKeyExistsAndValid"] --> D["_initialize_optimizer"]
  A["provideApiKey"] --> B["save_api_key_to_rc"]
  B --> C["clear API key cache"]
  C --> D
  D --> E["return success or error"]
Loading

Changes walkthrough 📝

Relevant files
Enhancement
cfapi.py
Add LSP-aware version check handling                                         

codeflash/api/cfapi.py

  • Log debug message instead of exit in LSP quiet mode
  • Return None for outdated CLI in quiet mode
  • +3/-0     
    env_utils.py
    Prefer shell config for API key in LSP                                     

    codeflash/code_utils/env_utils.py

  • In LSP quiet mode prefer shell config over env var
  • Swap retrieval order based on console.quiet flag
  • +6/-1     
    beta.py
    Implement LSP API key features                                                     

    codeflash/lsp/beta.py

  • Add ProvideApiKeyParams dataclass
  • Implement apiKeyExistsAndValid and provideApiKey handlers
  • Initialize optimizer only after valid API key
  • +50/-0   
    server.py
    Refactor LSP server init and logging                                         

    codeflash/lsp/server.py

  • Rename initializer to prepare_optimizer_arguments
  • Store args and defer optimizer instantiation
  • Add Debug mapping in show_message_log
  • +6/-4     
    Formatting
    shell_utils.py
    Quote API key in shell export                                                       

    codeflash/code_utils/shell_utils.py

    • Wrap API key in quotes in export line
    +1/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @github-actions
    Copy link

    github-actions bot commented Jul 9, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit a0ff82f)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Cache clearing

    provide_api_key calls get_user_id.cache_clear() but get_user_id is not decorated with a cache, so this may raise an AttributeError and lead to a misleading generic error.

    get_codeflash_api_key.cache_clear()
    get_user_id.cache_clear()
    Optimizer initialization

    _initialize_optimizer_if_valid uses server.args assuming it's set by prepare_optimizer_arguments, but if omitted, server.args could be None and cause failures when instantiating the optimizer.

    def _initialize_optimizer_if_valid(server: CodeflashLanguageServer) -> dict[str, str]:
        user_id = get_user_id()
        if user_id is None:
            return {"status": "error", "message": "api key not found or invalid"}
    
        from codeflash.optimization.optimizer import Optimizer
    
        server.optimizer = Optimizer(server.args)
        return {"status": "success", "user_id": user_id}
    Version check fallback

    In quiet mode, an outdated CLI version logs and returns None instead of exiting; ensure callers of get_user_id handle None correctly in the LSP flow.

    if console.quiet:  # lsp
        logger.debug(msg)
        return None
    sys.exit(1)

    @github-actions
    Copy link

    github-actions bot commented Jul 9, 2025

    PR Code Suggestions ✨

    Latest suggestions up to a0ff82f
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Guard cache_clear call

    Wrap the get_user_id.cache_clear() call in a guard to avoid an AttributeError when
    the function isn't cached. This ensures it only runs if the method exists.

    codeflash/lsp/beta.py [160]

    -get_user_id.cache_clear()
    +if hasattr(get_user_id, "cache_clear"):
    +    get_user_id.cache_clear()
    Suggestion importance[1-10]: 9

    __

    Why: The call to get_user_id.cache_clear() will raise an AttributeError if get_user_id isn't decorated with a cache, so guarding it prevents a runtime crash.

    High
    Security
    Escape quotes in export

    Escape any double quotes in the api_key before embedding it into the export line to
    avoid syntax errors or injection issues in shell RC files.

    codeflash/code_utils/shell_utils.py [45]

    -return f'{SHELL_RC_EXPORT_PREFIX}"{api_key}"'
    +sanitized_key = api_key.replace('"', '\\"')
    +return f'{SHELL_RC_EXPORT_PREFIX}"{sanitized_key}"'
    Suggestion importance[1-10]: 5

    __

    Why: Sanitizing api_key by escaping quotes prevents malformed export lines or injection issues in shell RC files, improving security with minimal overhead.

    Low

    Previous suggestions

    Suggestions up to commit 38e4e02
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Use correct server instance

    Use the passed-in _server parameter rather than the module-level server so you
    configure the correct LanguageServer instance.

    codeflash/lsp/beta.py [137]

    -server.optimizer = Optimizer(server.args)
    +_server.optimizer = Optimizer(_server.args)
    Suggestion importance[1-10]: 9

    __

    Why: Using the global server instead of the _server parameter will configure the wrong LanguageServer instance and break per-request state handling.

    High
    General
    Log validation exceptions

    Log the exception details before returning to aid debugging of unexpected failures
    during API key validation.

    codeflash/lsp/beta.py [139-140]

    -except Exception:
    +except Exception as e:
    +    logger.exception("Error validating API key")
         return {"status": "error", "message": "something went wrong while validating the api key"}
    Suggestion importance[1-10]: 5

    __

    Why: Adding exception logging improves debuggability for API key validation failures, though it doesn't alter core functionality.

    Low
    Log saving exceptions

    Capture and log the exception so any issues during saving the API key are recorded
    for troubleshooting.

    codeflash/lsp/beta.py [158-159]

    -except Exception:
    +except Exception as e:
    +    logger.exception("Error saving API key")
         return {"status": "error", "message": "something went wrong while saving the api key"}
    Suggestion importance[1-10]: 5

    __

    Why: Capturing and logging errors when saving the API key aids troubleshooting, but it's a minor enhancement rather than a critical fix.

    Low
    Consolidate optimizer import

    Move the Optimizer import to the module top-level and remove duplicate inline
    imports to reduce redundancy and improve performance.

    codeflash/lsp/beta.py [135-154]

    +# At the top of the module
     from codeflash.optimization.optimizer import Optimizer
     
    -# ... repeated inside each feature handler ...
    -from codeflash.optimization.optimizer import Optimizer
    +# Inside feature handlers, remove inline imports
    Suggestion importance[1-10]: 4

    __

    Why: Moving Optimizer imports to the module top-level reduces redundancy and slightly improves performance, but it's a low-impact refactor.

    Low

    @mohammedahmed18 mohammedahmed18 marked this pull request as ready for review July 11, 2025 18:43
    @github-actions
    Copy link

    Persistent review updated to latest commit a0ff82f

    @mohammedahmed18 mohammedahmed18 requested a review from KRRT7 July 13, 2025 10:21
    @misrasaurabh1 misrasaurabh1 requested a review from Saga4 July 14, 2025 20:54
    codeflash-ai bot added a commit that referenced this pull request Aug 6, 2025
    …d-submit-api-key-in-lsp`)
    
    The optimized code achieves a **10% speedup** through several key improvements:
    
    **1. Added Module-Level Import Fallback for CFAPI_BASE_URL**
    The optimized version imports `CFAPI_BASE_URL` at module load time with a try/except fallback, avoiding repeated import resolution during function calls. This eliminates the overhead of dynamic URL construction that was present in the original code.
    
    **2. Streamlined JSON Error Message Parsing**
    In the `make_cfapi_request` function, the error handling was simplified from:
    ```python
    if "error" in json_response:
        error_message = json_response["error"]
    elif "message" in json_response:
        error_message = json_response["message"]
    ```
    to:
    ```python
    error_message = json_response.get("error") or json_response.get("message", "")
    ```
    This reduces dictionary lookups and conditional branching.
    
    **3. Enhanced Response Handling in get_user_id**
    The optimized version adds explicit exception handling around `response.json()` parsing and caches `response.text` to avoid multiple property access. It also uses more descriptive variable names (`min_version_str`) to improve code clarity.
    
    **Performance Benefits by Test Case:**
    - **API key missing scenarios**: 12% faster (115ms → 102ms) - benefits most from the import optimization
    - **Basic success cases**: 1-2% faster - benefits from streamlined error handling
    - **Error response handling**: Minimal but consistent 0.2% improvements
    
    The optimizations are most effective for scenarios involving missing API keys or repeated function calls, where the module-level import resolution provides the largest performance gain.
    @codeflash-ai
    Copy link
    Contributor

    codeflash-ai bot commented Aug 6, 2025

    ⚡️ Codeflash found optimizations for this PR

    📄 11% (0.11x) speedup for get_user_id in codeflash/api/cfapi.py

    ⏱️ Runtime : 126 milliseconds 114 milliseconds (best of 11 runs)

    I created a new dependent PR with the suggested changes. Please review:

    If you approve, it will be merged into this PR (branch feat/verify-and-submit-api-key-in-lsp).

    codeflash-ai bot added a commit that referenced this pull request Aug 6, 2025
    …/verify-and-submit-api-key-in-lsp`)
    
    The optimization achieves a **50% speedup** by making two key improvements:
    
    **1. Eliminated redundant function definition**: The original code defined `read_api_key_from_shell_config()` locally even though it was already imported. The optimized version removes this duplicate definition and uses the imported function directly, reducing function call overhead.
    
    **2. Moved constant string outside function scope**: The long `api_secret_docs_message` string was defined inside `get_codeflash_api_key()` and recreated on every function call. The optimized version moves it to module level as a constant, eliminating repeated string allocation.
    
    **3. Added missing import**: The optimized code properly imports `get_cached_gh_event_data` from `codeflash.code_utils.env_utils`, which was missing in the original.
    
    These optimizations are particularly effective for **repeated API key lookups** (due to the `@lru_cache` decorator) and scenarios with **multiple error conditions**, as shown in the test results. The improvements reduce both memory allocation overhead and function resolution time, leading to consistent performance gains across all test cases without changing any functionality.
    @codeflash-ai
    Copy link
    Contributor

    codeflash-ai bot commented Aug 6, 2025

    ⚡️ Codeflash found optimizations for this PR

    📄 50% (0.50x) speedup for get_codeflash_api_key in codeflash/code_utils/env_utils.py

    ⏱️ Runtime : 4.91 microseconds 3.27 microseconds (best of 1 runs)

    I created a new dependent PR with the suggested changes. Please review:

    If you approve, it will be merged into this PR (branch feat/verify-and-submit-api-key-in-lsp).

    @Saga4 Saga4 merged commit 8fdc591 into main Aug 6, 2025
    19 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants